🤖 Kimi Code Review

Overall Assessment: Well-structured PR implementing Engine API scaffolding. Correctly keeps EL integration off the consensus critical path. Minor serialization bugs in wire types need fixing.

Critical Issues

1. Incorrect PayloadId serialization (crates/net/ethrex-client/src/types.rs:94-102)
   The #[serde(transparent)] attribute on PayloadId serializes the inner [u8; 8] as a JSON array of numbers, but the Engine API spec requires a 16-character hex string (e.g., "0x1234..."). This will cause deserialization failures when the EL returns a payload ID.

   • Fix: Remove #[serde(transparent)] and implement custom Serialize/Deserialize using to_hex() and a hex parser.

2. Missing hex encoding for fixed-size arrays (crates/net/ethrex-client/src/types.rs)
   PayloadAttributesV3.suggested_fee_recipient: [u8; 20] and Withdrawal.address: [u8; 20] will serialize as JSON arrays [0,1,2...] instead of hex strings. The spec requires DATA (hex) encoding for addresses.

   • Fix: Add a hex_fixed_array serde helper (or use serde_with::hex::Hex if that dependency is acceptable) for these fields, similar to the existing hex_bytes for Vec<u8>.

Security & Safety

3. JWT Secret File Permissions (crates/net/ethrex-client/src/auth.rs:76-80)
   JwtSecret::from_file reads the secret without checking file permissions. Ethereum clients conventionally require JWT secret files to have restrictive permissions (e.g., 0o600) to prevent unauthorized reads.
   • Suggestion: Add a permissions check (e.g., std::fs::metadata) and warn/error if the file is world-readable.

**Code Quality

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

🤖 Codex Code Review

Findings

1. High: Engine API address fields are serialized with the wrong JSON shape. In types.rs:35, types.rs:48, and types.rs:102, suggested_fee_recipient, Withdrawal.address, and fee_recipient are plain [u8; 20]. serde_json will emit those as byte arrays, not 0x... hex DATA strings, so engine_forkchoiceUpdatedV3(..., Some(payload_attributes)) and engine_newPayloadV3() are wire-incompatible with a spec-compliant EL.

2. Medium: Two valid Engine API responses cannot currently be deserialized. types.rs:57 models PayloadId as a transparent [u8; 8], but the wire format is a hex string like "0x0123...". types.rs:69 uses rename_all = "UPPERCASE" for PayloadStatusKind, which maps InvalidBlockHash to INVALIDBLOCKHASH instead of INVALID_BLOCK_HASH. A non-null payloadId or an INVALID_BLOCK_HASH response will surface as DeserializeResponse instead of a typed result.

3. Medium: hex_u256::deserialize can panic on malformed EL input. In types.rs:194, the code decodes arbitrary-length hex and then unconditionally copy_from_slices into a [u8; 32]. Anything longer than 32 bytes will panic and can take down the node. This should return a serde error, not crash the process.

4. Medium: The live FCU scaffold is sending beacon roots as execution block hashes. In lib.rs:231 through lib.rs:238, head(), safe_target(), and latest_finalized().root are passed straight into ForkChoiceState, but those are beacon roots, not EL block hashes. Until there is a real payload/header mapping, this should use an explicit dummy value (likely zeroes, which the comments/examples already describe) or keep FCU disabled; otherwise behavior depends on EL-specific handling of invalid hashes.

Open Questions / Assumptions

• I assumed the intent is still “wire scaffold only” until execution payloads exist. If so, Item 4 looks unintentional rather than a design change.
• The capability handshake result is logged but not acted on in main.rs:601. If “other EL” support matters, I’d expect unsupported V3 methods to disable per-slot FCU instead of warning forever.
• I couldn’t run cargo test here because the sandbox blocks dependency fetches and writable Cargo cache setup.

Outside the new EL client and tick hook, I didn’t see direct changes to LMD GHOST / 3SF-mini, attestation validation, justification/finalization, XMSS, or SSZ paths.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

Greptile Summary

This PR adds a new ethlambda-ethrex-client crate implementing JWT-authenticated Engine API connectivity (V3/Cancun subset) and wires it into the BlockChain actor as a fire-and-forget engine_forkchoiceUpdatedV3 call at every slot boundary.

• New ethrex-client crate: JwtSecret (HS256 mint, hex/file loaders), EngineClient (reqwest + JSON-RPC envelope), typed V3 wire types, and a hand-rolled TCP mock integration test.
• BlockChain actor hookup: optional EngineClient threaded in at startup; FCU fired via tokio::spawn at interval 0, never awaited, errors warn-logged.
• CLI flags: --execution-endpoint / --execution-jwt-secret with clap mutual-require; both absent means integration disabled with zero behavior change for existing nodes.

Confidence Score: 3/5

Safe for current FCU-only usage; two type definition bugs in types.rs will break wire format when payload building or INVALID_BLOCK_HASH responses are encountered

The JWT auth, transport, and blockchain-actor wiring are solid. However types.rs has two defects that matter as soon as deferred milestones are picked up: PayloadStatusKind would produce a deserialization error on any INVALID_BLOCK_HASH EL response, and the [u8; 20] address fields emit integer arrays instead of hex strings, guaranteed to be rejected by the EL when payload building is attempted.

crates/net/ethrex-client/src/types.rs needs the PayloadStatusKind rename attribute corrected and hex serializers added to the three [u8; 20] address fields before payload flow is wired

Sequence Diagram

sequenceDiagram
    participant CLI as bin/ethlambda
    participant EC as EngineClient
    participant EL as ethrex Engine API

    Note over CLI: startup
    CLI->>EC: new(endpoint, JwtSecret)
    CLI->>EC: exchange_capabilities
    EC->>EL: POST engine_exchangeCapabilities
    EL-->>EC: capability list
    EC-->>CLI: Ok or warn and continue

    Note over CLI: BlockChain spawned with optional client

    loop every slot at interval 0
        CLI->>EC: notify_execution_layer via tokio::spawn
        EC->>EC: sign fresh JWT
        EC->>EL: POST engine_forkchoiceUpdatedV3
        EL-->>EC: SYNCING scaffold response
        EC-->>CLI: logged, never blocks consensus
    end

Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 4
crates/net/ethrex-client/src/types.rs:68-76
`UPPERCASE` rename collapses multi-word variant names without inserting underscores, so `InvalidBlockHash` serializes/deserializes as `"INVALIDBLOCKHASH"` rather than the Engine API spec value `"INVALID_BLOCK_HASH"`. Any EL response carrying that status will fail to deserialize, surfacing as an `EngineClientError::DeserializeResponse` instead of the correct typed variant. Using `SCREAMING_SNAKE_CASE` fixes all five variants simultaneously.

```suggestion
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "SCREAMING_SNAKE_CASE")]
pub enum PayloadStatusKind {
    Valid,
    Invalid,
    Syncing,
    Accepted,
    InvalidBlockHash,
}
```

### Issue 2 of 4
crates/net/ethrex-client/src/types.rs:35
`[u8; 20]` with no custom serializer is emitted by `serde_json` as a JSON integer array (`[0,1,2,...]`), not the `"0x..."` DATA string the Engine API spec requires. The same issue affects `fee_recipient` in `ExecutionPayloadV3` (line 102) and `address` in `Withdrawal` (line 48). Both `engine_newPayloadV3` and any FCU call that passes `PayloadAttributesV3` will produce a malformed request body that the EL will reject.

### Issue 3 of 4
docs/plans/engine-api-integration.md:168-170
The References section embeds absolute paths from the author's local machine (`/Users/pablodeymonnaz/Lambda/ethrex/...`). These should be replaced with GitHub URLs so other contributors can follow them. The same issue appears in the "Starting state" section (line 33).

### Issue 4 of 4
bin/ethlambda/src/main.rs:321-325
The warning message "will keep retrying on each tick" implies `engine_exchangeCapabilities` is called on every slot. In practice only `engine_forkchoiceUpdatedV3` runs on each tick; the capability handshake is one-shot at startup. The comment should describe what actually happens, e.g. "EL will still receive FCU calls; capability list is unknown".

_{Reviews (1): Last reviewed commit: "feat: integrate with ethrex over Engine ..." | Re-trigger Greptile}

🤖 Claude Code Review

Now I have a thorough view of all the critical files. Let me compile the review.

Review: feat: integrate with ethrex over the Engine API

Overview

This PR adds the Engine API plumbing between ethlambda and an execution layer (ethrex). It is deliberately scoped to Option B ("scaffold") — the JWT/transport/tick-wiring layer — while deferring actual payload flow until leanSpec defines ExecutionPayload. That scope is appropriate and the architecture is sound. A few correctness issues in the type layer need addressing before this merges.

Correctness Issues

1. PayloadStatusKind::InvalidBlockHash will not round-trip correctly

types.rs, PayloadStatusKind:

#[serde(rename_all = "UPPERCASE")]
pub enum PayloadStatusKind {
    ...
    InvalidBlockHash,
}

serde(rename_all = "UPPERCASE") converts each variant by uppercasing the entire name, so InvalidBlockHash → "INVALIDBLOCKHASH". The execution-apis spec requires "INVALID_BLOCK_HASH". Any response from the EL with that status will fail deserialization silently (falling through as an UnknownVariant error). Fix:

#[serde(rename = "INVALID_BLOCK_HASH")]
InvalidBlockHash,

2. [u8; 20] address fields serialize as integer arrays, not hex strings

types.rs, PayloadAttributesV3, ExecutionPayloadV3, and Withdrawal:

pub suggested_fee_recipient: [u8; 20],   // PayloadAttributesV3
pub fee_recipient: [u8; 20],              // ExecutionPayloadV3
pub address: [u8; 20],                    // Withdrawal

serde_json serializes [u8; N] as [0, 0, 0, ...], not "0x000...000". The Engine API spec requires DATA encoding (hex with 0x prefix). This is dormant in M4 because forkchoice_updated_v3 is only called with payload_attributes: None and ExecutionPayloadV3 is not yet wired — but the types are part of the crate's public API and they will silently produce malformed JSON the moment they're exercised. The bug should be fixed now alongside the type definitions, not left for M6.

The fix is a newtype or a custom hex_address serde module (mirroring hex_u64/hex_bytes) applied via #[serde(with = "hex_address")].

Minor Issues

3. Misleading log message in build_execution_client

main.rs:

Err(err) => warn!(
    %err,
    "EL capability handshake failed (will keep retrying on each tick)"
),

exchange_capabilities is called exactly once, at startup. The per-tick call is forkchoice_updated_v3, not the capability handshake. The parenthetical is inaccurate and will confuse operators debugging a failed handshake. Suggest: "EL capability handshake failed; FCU calls will still be attempted each slot".

4. No HTTP status check before body parsing in rpc_call

client.rs:

let raw = self
    .http
    .post(&self.url)
    ...
    .send()
    .await?
    .text()
    .await?;

If the EL returns a 401 (wrong JWT), 403, or 500, the body may not be valid JSON-RPC. The error will surface as EngineClientError::DeserializeResponse with no indication of the HTTP status code, making debugging harder. Adding .error_for_status() before .text() would give a cleaner error path for authentication/server failures.

5. Fixed id: 1 in JSON-RPC requests

client.rs, JsonRpcRequest:

id: 1,

Fine for fire-and-forget sequential calls, but if concurrent requests are ever issued (possible if two rapid tick intervals fire before the first completes given the 8s timeout), request/response correlation by id breaks. Not a bug today given the tokio::spawn-based fire-and-forget model, but worth noting for future parallelism.

6. JwtSecret stores bytes as Vec<u8>

auth.rs:

pub struct JwtSecret {
    bytes: Vec<u8>,
}

Since from_bytes enforces exactly 32 bytes, the type could be [u8; 32], which eliminates the heap allocation, makes Clone cheaper (stack copy vs. heap clone), and opens the door to deriving Copy. The WrongLength variant would still exist but would only be reachable from from_bytes.

Positive Observations

• Fire-and-forget via tokio::spawn is exactly right. Consensus ticks are never awaited on EL state.
• Fresh JWT per request with sign_now() is spec-correct; the sign(iat) / sign_now() split makes deterministic testing clean.
• Graceful degradation: startup failures return None and log at error!/warn! rather than crashing the node. Correct.
• ForkChoiceState comment explicitly calls out that beacon roots are used as EL block hashes in M4 scaffold mode. Helps future reviewers.
• Wire smoke test (tests/wire_smoke.rs) uses a hand-rolled TcpListener rather than a mock crate — no extra dependencies and the assertions cover both the JWT header and the JSON-RPC body shape.
• Both-or-neither CLI enforcement via #[arg(requires = ...)] is clean and avoids partial configuration.
• hex_u256 serializer correctly handles the all-zeros case ("0x0") and strips leading zero nibbles per the QUANTITY spec.
• ethrex-client as a separate crate (not folded into crates/net/rpc) is the right call given the orthogonal dependency footprint.

Summary

The transport and auth foundations are solid. Two correctness issues need fixing before M6 exercises the types:

• Item 1 (InvalidBlockHash rename) is a compile-silent deserialization bug that will manifest in any network that sends that status.
• Item 2 ([u8; 20] address serialization) is a wire-format bug dormant in M4 but guaranteed to surface in M6; fixing it now is cheaper than discovering it mid-integration.

Items 3–6 are non-blocking improvements.

Automated review by Claude (Anthropic) · sonnet · custom prompt